-
Notifications
You must be signed in to change notification settings - Fork 63
feat: Add TokenX integration and fix FabricX transaction mismatch #1101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a comprehensive TokenX integration for FabricX and fixes a critical transaction mismatch issue in the endorsement process. The main purpose is to provide a working token management system while resolving bugs that prevented proper endorsement validation.
Key Changes:
- Fixed FabricX transaction endorsement mismatch by using received RWSet bytes directly instead of re-serializing
- Fixed sidecar port configuration to use dynamic ports instead of hardcoded values
- Added comprehensive TokenX integration with support for issuing, transferring, redeeming, and swapping tokens
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| platform/fabricx/core/transaction/transaction.go | Core fix: uses received RWSet bytes for endorsement to avoid namespace version mismatches |
| platform/fabricx/core/vault/interceptor.go | Enhanced logging for debugging RWSet serialization and namespace versions |
| platform/fabric/services/endorser/endorsement.go | Added detailed error logging for endorsement result mismatches |
| integration/nwo/fabricx/extensions/scv2/notificationservice.go | Modified to accept dynamic port parameter |
| integration/nwo/fabricx/extensions/scv2/ext.go | Passes dynamic sidecar port to notification service configuration |
| integration/fabricx/tokenx/views/*.go | New token operation views (issue, transfer, redeem, swap, balance, auditor, approver) |
| integration/fabricx/tokenx/states/states.go | Data models for tokens, transaction records, and swap proposals |
| integration/fabricx/tokenx/topology.go | Network topology definition with issuer, auditor, and owner nodes |
| integration/fabricx/tokenx/api/*.go | REST API handlers and OpenAPI specification |
| integration/fabricx/tokenx/*.md | Comprehensive documentation including walkthrough, specification, and task tracking |
| integration/fabricx/tokenx/*_test.go | Integration test suite using Ginkgo |
| Makefile | Added tokenx integration test target and updated linter installation path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (h *redeemHandler) HandleRequest(ctx *server.ReqContext) (interface{}, int) { | ||
| req := ctx.Query.(*RedeemRequest) | ||
|
|
||
| amount := states.TokenFromFloat(100) // TODO: parse req.Amount |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded TODO comment indicates incomplete implementation. The amount parsing from the request is not implemented, which means all redeem requests will attempt to redeem the same hardcoded amount (100) regardless of the requested amount in the API call.
| amount := states.TokenFromFloat(100) // TODO: parse req.Amount | |
| amount := states.TokenFromFloat(req.Amount) |
| logger.Infof("Querying owner history: type=%s, txType=%s", h.TokenType, h.TxType) | ||
|
|
||
| // Get current owner identity | ||
| fns, err := fabric.GetDefaultFNS(ctx) | ||
| assert.NoError(err, "failed getting FNS") | ||
| _ = fns.LocalMembership().DefaultIdentity() | ||
|
|
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The owner variable is assigned but never used. This suggests the history query implementation is incomplete and currently returns empty results.
| logger.Infof("Querying owner history: type=%s, txType=%s", h.TokenType, h.TxType) | |
| // Get current owner identity | |
| fns, err := fabric.GetDefaultFNS(ctx) | |
| assert.NoError(err, "failed getting FNS") | |
| _ = fns.LocalMembership().DefaultIdentity() | |
| // Get current owner identity | |
| fns, err := fabric.GetDefaultFNS(ctx) | |
| assert.NoError(err, "failed getting FNS") | |
| owner := fns.LocalMembership().DefaultIdentity() | |
| logger.Infof("Querying owner history for owner [%s]: type=%s, txType=%s", owner, h.TokenType, h.TxType) |
Makefile
Outdated
| install-linter-tool: ## Install linter in $(go env GOPATH)/bin | ||
| @echo "Installing golangci Linter" | ||
| @curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.4.0 | ||
| @curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b ~/.local/bin v2.4.0 |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The golangci-lint installer path should use a shell variable expansion $(shell go env GOPATH)/bin instead of the literal string ~/.local/bin to ensure consistency across different development environments and to match the pattern used in line 41.
| @curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b ~/.local/bin v2.4.0 | |
| @curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(shell go env GOPATH)/bin v2.4.0 |
| var serErr error | ||
| rawTx, serErr = rwset.Bytes() | ||
| if serErr != nil { | ||
| return nil, errors.WithMessagef(serErr, "error serializing rws for [%s]", t.ID()) |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable serErr is unnecessarily introduced. The error can be returned directly using err as in the surrounding code, which is more consistent with the codebase style.
| var serErr error | |
| rawTx, serErr = rwset.Bytes() | |
| if serErr != nil { | |
| return nil, errors.WithMessagef(serErr, "error serializing rws for [%s]", t.ID()) | |
| rawTx, err = rwset.Bytes() | |
| if err != nil { | |
| return nil, errors.WithMessagef(err, "error serializing rws for [%s]", t.ID()) |
| Amount: i.Amount, | ||
| Owner: i.Recipient, | ||
| IssuerID: "issuer", // Simple identifier | ||
| CreatedAt: time.Time{}, |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CreatedAt field is initialized with a zero time value time.Time{} instead of the current time using time.Now(). This is inconsistent with other token creation in the codebase (see transfer.go line 86, 99) and will result in tokens having an invalid timestamp.
| CreatedAt: time.Time{}, | |
| CreatedAt: time.Now(), |
| // For now, we demonstrate the pattern | ||
| _ = qs | ||
| _ = vault | ||
| _ = owner |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variables qs, vault, and owner are assigned but never used. This suggests the balance query implementation is incomplete and currently returns empty results.
| // For now, we demonstrate the pattern | |
| _ = qs | |
| _ = vault | |
| _ = owner | |
| // For now, we demonstrate the pattern. Currently, we only log the context. | |
| logger.Infof( | |
| "[BalanceView] Query context - owner: %+v, queryService: %T, vault: %T", | |
| owner, qs, vault, | |
| ) |
| TokenType: i.TokenType, | ||
| Amount: i.Amount, | ||
| To: i.Recipient, | ||
| Timestamp: time.Time{}, |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Timestamp field is initialized with a zero time value time.Time{} instead of the current time using time.Now(). This is inconsistent with other transaction records in the codebase (see transfer.go line 112) and will result in records having invalid timestamps.
| req := ctx.Query.(*IssueRequest) | ||
|
|
||
| // Convert amount string to uint64 (would parse decimal in production) | ||
| amount := states.TokenFromFloat(1000) // TODO: parse req.Amount |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded TODO comment indicates incomplete implementation. The amount parsing from the request is not implemented, which means all issue requests will create tokens with the same hardcoded amount (1000) regardless of the requested amount in the API call.
| func (h *transferHandler) HandleRequest(ctx *server.ReqContext) (interface{}, int) { | ||
| req := ctx.Query.(*TransferRequest) | ||
|
|
||
| amount := states.TokenFromFloat(100) // TODO: parse req.Amount |
Copilot
AI
Jan 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded TODO comment indicates incomplete implementation. The amount parsing from the request is not implemented, which means all transfer requests will attempt to transfer the same hardcoded amount (100) regardless of the requested amount in the API call.
|
@gutama Thank you for creating this PR. Can you please have a look at the DCO check and fix it. |
Signed-off-by: gutama <ginanjar.utama@gmail.com>
25e23e2 to
d534a52
Compare
|
DCO check passed, thank you |
|
I'm wondering if the TokenX app should be better positioned in the fabric-samples rather than here. |
|
@gutama , is this PR fully generated by AI? |
|
Actually at first I only want to learn how to use fabricx by extending simple and iou sample, so TokenX uses FabricX and FSC state management directly. Somehow I can't run integration test successfully that's why I put PR, because the code need changes in other places. Fully generated by AI and a lot of debugging. |
Signed-off-by: Ginanjar Utama <ginanjar.utama@gmail.com>
| SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we really should add this api package as part of an integration test here. For a real FSC-based application, it makes sense to me, however, as part of the integration test suite I believe we aim to keep the tests small.
| Issue | ||
| } | ||
|
|
||
| func (i *IssueView) Call(ctx view.Context) (interface{}, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that this view is called via the integration tests. @gutama Do you plan to also add tests for the other views, including, transfer, redeem, etc ... ?
| func (e *Extension) GenerateArtifacts() { | ||
| generateQSExtension(e.network) | ||
| generateNSExtension(e.network) | ||
| generateNSExtension(e.network, e.sidecar.Ports[fabric_network.ListenPort]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good improvement!
| @@ -0,0 +1,1008 @@ | |||
| # TokenX - Token Management System Specification | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this a great spec! However, I have the feeling it's a bit beyond the scope of an integration test. I could think about create a dedicated lab to continue development of this application, or work towards a "fabric-x" sample. What do you think?
| ### 1. Endorsement Mismatch (Fixed) | ||
|
|
||
| **Issue**: The FabricX RWSet serialization includes local namespace versions. When the approver node re-serialized the RWSet, it used its own local versions which differed from the issuer's, causing a hash mismatch. | ||
|
|
||
| **Fix**: Modified `platform/fabricx/core/transaction/transaction.go` to check if `t.RWSet` is populated (received from issuer) and use it directly instead of re-serializing. | ||
|
|
||
| ### 2. Sidecar Port Mismatch (Fixed) | ||
|
|
||
| **Issue**: The integration test hung at "Post execution for FSC nodes...". Debugging revealed the Sidecar container was launching on a dynamic port (e.g., 5420), but the client configuration was hardcoded to connect to `5411`. | ||
|
|
||
| **Fix**: | ||
| - Updated `integration/nwo/fabricx/extensions/scv2/notificationservice.go`: `generateNSExtension` now accepts a port argument. | ||
| - Updated `integration/nwo/fabricx/extensions/scv2/ext.go`: `GenerateArtifacts` retrieves the correct `fabric_network.ListenPort` from the sidecar configuration and passes it to `generateNSExtension`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job identifying these issue. Let's work on getting these fixes in.
|
@gutama Thanks again for your PR. I left a few comments (see above). I really like your proposal for another integration test that demonstrates a full token system built with FPC and Fabric-x, however, I am also hesitating if the FSC integration tests are the best home for this application. Ideally, we can refine the scope of this PR and remove functionality which is not yet tests (e.g., |
No description provided.